-
Notifications
You must be signed in to change notification settings - Fork 106
Add reusable ownable module and refactor network fungible faucet to use it #2228
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
onurinanc
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @afa7789 for this work! I have few comments from my side. I believe @mmagician would provide a more detailed feedback related to ownable logic.
Also, for the folder structure such as how we would like to have the utils under the standards.
Additionally, from the discussion here: OpenZeppelin/miden-confidential-contracts#34 (comment)
Is it possible to design an address, which is not usable for anyone such as address(0)?
| # | ||
| # See the `NetworkFungibleFaucet` Rust type's documentation for more details. | ||
|
|
||
| use miden::standards::utils::ownable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is nice!
We might want to have type of utils such as access utils or token utils. Also, we can provide some documents under the utils. So, I think we can go one step further and have
standards::utils::access::ownable
crates/miden-standards/asm/standards/faucets/network_fungible.masm
Outdated
Show resolved
Hide resolved
|
@mmagician can you also take a look! ? :) |
I did a full refactor in branch openzeppelin/ownable_dos - moved all ownership logic inline to network_fungible.masm (no external module calls). Same result: set_item fails with "storage slot not found". Root cause: When a note script calls an account procedure via call, the kernel's authenticate_account_origin sees the note script hash as the caller, not an account procedure. This blocks all storage writes from note scripts, regardless of where the code is defined. What works: Owner verification for minting (read-only). What doesn't work: transfer_ownership (requires write). This appears to be a fundamental Miden limitation. @mmagician is there a way to workaround it ? |
| # | ||
| # See the `NetworkFungibleFaucet` Rust type's documentation for more details. | ||
|
|
||
| use miden::standards::utils::access::ownable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: since we are using the fully-qualified path for exports, we shouldn't need to add this import.
An alternative could be to do something like this:
use miden::standards::utils::access::ownable
pub use ownable::transfer_ownership
But we kind of been using fully-qualified paths in such situations - so, probably should stick with it.
42c387a to
176eb62
Compare
crates/miden-standards/asm/standards/faucets/network_fungible.masm
Outdated
Show resolved
Hide resolved
crates/miden-standards/asm/standards/faucets/network_fungible.masm
Outdated
Show resolved
Hide resolved
| # => [pad(16)] | ||
|
|
||
| # ---- Write zero AccountId to storage ---- | ||
| push.0.0.0.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we can model zero address not to be usable anyone, and we can name it AddressZero in the kernel. @mmagician
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we can assume that nobody will be able to come up with address that is all zeros (if they can, then collision-resistance of the hash function is broken, and we have bigger problems).
We could also add this in the kernel - but not sure where yet (and maybe that should be a separate issue).
For now, I would create a constant in this file and then just do something like push.ZERO_ADDRESS.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could also add this in the kernel - but not sure where yet
I don't think there would be any benefits to doing this in the kernel. In Ethereum this is not defined at the protocol level, but rather it's assumed (by preimage resistance) that no one will have the key to that account.
bobbinth
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Thank you! I some comments inline - mostly minor.
| # => [pad(16)] | ||
|
|
||
| # ---- Write zero AccountId to storage ---- | ||
| push.0.0.0.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we can assume that nobody will be able to come up with address that is all zeros (if they can, then collision-resistance of the hash function is broken, and we have bigger problems).
We could also add this in the kernel - but not sure where yet (and maybe that should be a separate issue).
For now, I would create a constant in this file and then just do something like push.ZERO_ADDRESS.
crates/miden-standards/asm/standards/faucets/network_fungible.masm
Outdated
Show resolved
Hide resolved
crates/miden-standards/asm/standards/faucets/network_fungible.masm
Outdated
Show resolved
Hide resolved
crates/miden-standards/asm/standards/faucets/network_fungible.masm
Outdated
Show resolved
Hide resolved
|
@bobbinth @onurinanc @mmagician I changed/solved the issues/suggestions highlighted here in the PR. |
bobbinth
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Thank you. I left some more comments inline. Could you also please a changelog entry for this PR?
crates/miden-standards/asm/standards/faucets/network_fungible.masm
Outdated
Show resolved
Hide resolved
crates/miden-standards/asm/standards/faucets/network_fungible.masm
Outdated
Show resolved
Hide resolved
|
@bobbinth done ! |
|
@afa7789 thank you! Seems like there are some outstanding merge conflicts. Could you resolve them? |
bfd268a to
067df65
Compare
bobbinth
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Thank you! I primarily focused on non-test code.
Maybe @onurinanc wants to take another look?
* chore: rename auth to auth_scheme * chore: split up asm/account_components * chore: only use recursive masm finder * chore: rename to remove "_recursive" * chore: align build.rs files * chore: adjust comment
* Update memory.rs * Update memory.masm * Update crates/miden-lib/src/transaction/memory.rs Co-authored-by: Philipp Gackstatter <PhilippGackstatter@users.noreply.github.com> * Update crates/miden-lib/src/transaction/memory.rs Co-authored-by: Philipp Gackstatter <PhilippGackstatter@users.noreply.github.com> * Update memory.rs * Update test_fpi.rs * Update memory.masm * Update kernel_procedures.rs * fix: make format * Update memory.masm * Update account.masm * Update memory.rs * Update account.masm --------- Co-authored-by: Philipp Gackstatter <PhilippGackstatter@users.noreply.github.com>
* feat: move standard note scripts into standard library Move note script logic from standalone files in `note_scripts/` directory to `miden::standards::notes` namespace modules. This enables dynamic access to script roots via `procref` instead of hardcoding values. Changes: - Add new modules under `standards/notes/`: p2id, p2ide, swap, burn, mint - Each module contains a `pub proc main` with the script logic - Original note script files now serve as minimal wrappers calling `exec.<note>::main` Closes 0xMiden#2243 * chore: add changelog entry for note scripts refactoring * docs: fix namespace references in note script documentation * chore: update docs contracts->standards --------- Co-authored-by: Marti <marti@miden.team>
Previously, the Minimum Supported Rust Version (MSRV) check ran on every pull request and push to next. This was expensive because cargo-msrv downloads additional Rust toolchains to verify compatibility. Now the MSRV check only runs: - On push to main/next (in release-plz-dry-run.yml) - When publishing a release (in release-plz.yml) This makes PR CI faster and uses less memory. Any MSRV issues will still be caught before publication, and can be fixed locally using the scripts/check-msrv.sh script. Changes: - Delete .github/workflows/msrv.yml - Add MSRV check steps to release-plz-dry-run.yml - Add MSRV check steps to release-plz.yml
* feat: Define `NoteTag` to wrap arbitrary `u32` * feat: Rework note tag docs and constructors * chore: Remove use case constructors and refactor swap tag * feat: Remove note tag validation * feat: Rename account ID conversion to account target * feat: Update note tag docs * chore: add changelog * chore: Make `NoteMetadata::new` infallible * chore: address review comments on docs * chore: remove outdated note tag rules in metadata docs * chore: remove `create_mock_notes` (0xMiden#2236) * chore: Rename `create_random_note` to `*_default_*` and add pub helper * chore: replace create_mock_notes in test_epilogue * chore: Remove `create_mock_notes` --------- Co-authored-by: Bobbin Threadbare <43513081+bobbinth@users.noreply.github.com>
* feat: Implement `NoteAttachment` * chore: Precompute `NoteHeader` in `PartialNote` * feat: Remove aux, exec hint from `NoteMetadata`; add attachment * feat: refactor output note memory layout and metadata building * chore: adapt tx event extraction to new output_note::create stack state * chore: prepare `compute_output_notes_commitment` for attachment hashing * chore: Update input notes commitment to include attachment * chore: include attachment in output notes commitment * chore: return attachment and header from `input_note_get_metadata` * chore: return attachment and header from `output_note_get_metadata` * fix: compilation errors in `miden-testing` * chore: regenerate kernel procedure hashes * chore: add changelog * chore: rename attachment_types to attachment_type_info for clarity * chore: Move content to word encoding to method * chore: rename `Raw` -> `Word` and `Commitment` -> `Array` * chore: Rename `NoteAttachmentCommitment` to `*Array`
* feat: Implement `output_note_set_attachment` kernel API * feat: Implement `miden::protocol` set_attachment wrappers * feat: Emit event during `set_attachment` for tx host * chore: add tests for `set_attachment` APIs * chore: regenerate kernel procedure hashes * chore: add changelog * chore: rename `Raw` -> `Word` and `Commitment` -> `Array` * fix: renamed attachment content variants * chore: make attachment content type constants public * chore: move note ptr to idx conversion to helper * chore: merge attachment validation procedures into one * chore: simplify setting output note attachment type info * chore: make `set_attachment` public * fix: commitment to array rename * chore: require attachment_type == 0 when content type is None * chore: enforce none / attachment type restriction in `NoteAttachment`
) * feat: Implement `NetworkAccountTarget` * chore: Let SPAWN note create output notes with attachment * chore: test creating note with NetworkAccountTarget attachment * chore: add changelog * fix: don't use super::* import * feat: Add `WellKnownNoteAttachment` * chore: Use 8-bit tag for exec hint; remove `AfterBlockNumber` * chore: improve network account target error
…::create` (0xMiden#2260) * chore: remove aux and exec hint from protocol::output_note::create * fix: tests in `test_epilogue.rs` * fix: update calls to `output_note::create` in note tests * fix: update calls to `output_note::create` in tx and account_delta test * fix: remove aux and exec hint from `send_note_body` * chore: remove exec hint and aux from swap note * chore: update MINT note to new output_note::create signature * fix: remove aux/exec hint from faucet tests * fix: replace aux with attachment in p2id constructors * fix: replace aux with attachment in mint, burn, p2ide notes * chore: remove aux and exec hint metadata methods * chore: remove aux/exec hint from `NoteBuilder` * chore: add changelog * feat: Take attachment param in `faucets::distribute` * chore: update MINT note to new faucets::distribute params * feat: allow attachment for payback note in SWAP note * fix: calls to `distribute` in tests; remove mention of aux * Revert "feat: Take attachment param in `faucets::distribute`" This reverts commit 8c27281. * chore: return note_idx from `distribute` * Partially revert chore: update MINT note to new faucets::distribute params * chore: update MINT note with new note inputs memory offets * chore: set attachment in MINT note * Partially revert fix: calls to `distribute` in tests; remove mention of aux * feat: Add attachment to note in `send_body` * chore: remove unnecessary `push.0`
…xMiden#2268) * feat: Pad elements in `NoteAttachmentArray` before committing * chore: Rename `NoteAttachmentType` -> `NoteAttachmentScheme` * chore: rename "attachment type info" to "attachment kind scheme" * chore: Rename `NoteAttachmentContentType` -> `NoteAttachmentKind` * chore: rename attachment scheme untyped to unknown * chore: Swap kind and scheme in procedure signatures * chore: add changelog * Revert "feat: Pad elements in `NoteAttachmentArray` before committing" This reverts commit cfe9702. * chore: rename `NoteAttachmentScheme::unknown` to `none` * fix: more content type -> attachment kind renames
…and integrate into network fungible faucet
…th default attachments in faucet tests
c905269 to
1d1bdf3
Compare
decbd4d to
c8bcbfe
Compare
|
I had to force push to sign all the commits, started without signing. |
mmagician
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with minor adjustments
Co-authored-by: Marti <marcin.gorny.94@protonmail.com>
Co-authored-by: Marti <marcin.gorny.94@protonmail.com>
Co-authored-by: Marti <marcin.gorny.94@protonmail.com>
mmagician
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the quick adjustments!

This PR creates a new reusable
ownablemodule that centralizes ownership management functionality for account components.The network fungible faucet has been refactored to use this module instead of implementing ownership logic inline, replacing the local
is_ownerprocedure withownable::check_ownerand updating the storage slot to use the standardownable::owner_configslot name. Thenetwork_fungible_faucetcomponent now also exportstransfer_ownershipfrom the ownable module.This change improves code reusability and maintainability by standardizing ownership logic across components. New tests have been added to verify ownership checks work correctly, and all existing tests continue to pass.
OpenZeppelin/miden-confidential-contracts#34 ( regarding this discussion ).